Skip to content

Parenthesize MIN/MAX macro arguments - #9928

Open
akx wants to merge 1 commit into
python-pillow:mainfrom
akx:hygienize-macros
Open

Parenthesize MIN/MAX macro arguments#9928
akx wants to merge 1 commit into
python-pillow:mainfrom
akx:hygienize-macros

Conversation

@akx

@akx akx commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

#9921 replaced duplicated variants of MIN/MAX macros with the variant that has certain subtle precedence issues, see e.g. the trivial example

#define MAX_BAD(a, b)  (a > b ? a : b)
#define MAX_GOOD(a, b) ((a) > (b) ? (a) : (b))

int main(void) {
    int x = 1, y = 10, z = 4;   /* y | z == 14 */
    printf("%d\n", MAX_BAD(x, y | z));   /* prints 1  */
    printf("%d\n", MAX_GOOD(x, y | z));  /* prints 14 */
}

or MIN(a, flag ? x : y) which would expand to (a < flag ? x : y ? a : flag ? x : y), which parses as (a < flag) ? x : (y ? a : (flag ? x : y)), which is probably not what the programmer expects.

While right now it looks (quick visual grep) like there were no call sites affected, I think it's a good idea to have the predictable variant of these macros, instead of the subtly wrong one.

@akx

akx commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

See #9807 (comment) and #9807 (comment), related.

fallenmi
fallenmi approved these changes Aug 30, 2026
View reviewed changes

@fallenmi fallenmi left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defensive parentheses fix both precedence classes I could reproduce. Against the production ImagingUtils.h, exact PR base cacb5139bf9abe70afe2cd533b319d5c66a48867 computes MAX(1, 10 | 4) as 1 and MIN(5, 1 ? 2 : 9) as 5; Clang with -Werror also rejects the first expansion. Exact head a34b2219fd573b69517d9468e04b8ee2992b80cb produces the expected 14 and 2 and compiles cleanly with -std=c11 -Wall -Wextra -Werror.

I also merged that exact head cleanly into current protected main e41083f383c9cd3db95de52564cc0b6452313d4a and repeated the oracle. The focused suites covering all current macro-consumer groups (Convert, BoxBlur, QuantOctree, and TiffDecode) passed on both exact head and the current-main merge: 476 passed, 4 unrelated platform/optional-data skips in each tree. git diff --check is clean. At final recheck, all 55 exact-head check runs and all 11 check suites were successful, as were both legacy statuses.

Reviewed with OpenAI Codex assistance; exact current-head and current-main-merge behavior was reproduced locally. I found no blocker in the measured scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@fallenmi fallenmi fallenmi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@akx @fallenmi